Skip to content

fix(mysql): bound ORC roleProbe client calls#3117

Draft
weicao wants to merge 10 commits into
mainfrom
henry/task82-orc-roleprobe-budget
Draft

fix(mysql): bound ORC roleProbe client calls#3117
weicao wants to merge 10 commits into
mainfrom
henry/task82-orc-roleprobe-budget

Conversation

@weicao

@weicao weicao commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Problem

Focused Orchestrator acceptance evidence showed repeated kbagent role-probe timeouts and an unbounded switchover path. The old scripts also allowed failed Orchestrator prechecks to be confused with normal topology output.

Changes

  • scope a 5-second role-probe period/timeout to Orchestrator ComponentDefinitions;
  • bound every role-probe client call to 4 seconds and fail closed on timeout, non-zero rc, empty master output, or a Pod missing from topology;
  • bound switchover prechecks and preserve hard failure for Orchestrator command errors;
  • verify switchover convergence over a bounded window using both Orchestrator topology and MySQL read-only flags;
  • emit retry-safe diagnostic context when topology does not converge;
  • keep the non-Orchestrator syncerctl getrole probe at 1 second.

Rebase Resolution

The branch was rebased onto current main at exact head 86c7cea083adc5031b5ed8d2c317a6c552046f8f. Two conflicts in orc-switchover.sh were resolved by retaining main's fail-closed Orchestrator rc checks together with this branch's bounded precheck and post-check convergence logic.

Validation

  • bash -n on role-probe and switchover scripts: PASS
  • helm lint addons/mysql: PASS
  • full chart render: PASS
  • render shows ORC roleProbe 5s/5s and preserves the regular 1s/1s probe
  • git diff --check: PASS
  • exact-head PR CI is green except the currently running ShellSpec job

Evidence Boundary

This is source, syntax, render, and CI evidence. Focused runtime replay remains N=0, so the PR stays draft and does not establish switchover PASS, full-suite PASS, or release readiness.

@codecov-commenter

codecov-commenter commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 201 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (b55707f) to head (86c7cea).

Files with missing lines Patch % Lines
...ddons/mysql/scripts-ut-spec/orc_switchover_spec.sh 0.00% 137 Missing ⚠️
...ddons/mysql/scripts-ut-spec/orc_role_probe_spec.sh 0.00% 64 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #3117    +/-   ##
======================================
  Coverage   0.00%   0.00%            
======================================
  Files        147     149     +2     
  Lines      23282   23483   +201     
======================================
- Misses     23282   23483   +201     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@weicao

weicao commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed head a0fa6ec8 (delta vs base helios/mysql-fixes-integration). Pre-flight gates + 8-class design-contract walk. Overall: the switchover verify-while-client-runs design is right and matches the field evidence (takeover succeeded at Orchestrator while Ops timed out on convergence — positive state verification instead of trusting client rc is exactly the correct fail-closed shape). Structured retry-safe stderr diagnostics + mirrored /tmp/switchover.log also close the evidence gap every focused #3078 run has hit. ShellSpec coverage is meaningful (stderr-noise-tolerant parsing, non-zero-client-but-converged case). Two blockers, two verify-flags, minor notes:

Blocker 1 — apps.kubeblocks.io/skip-immutable-check: "true" in mysql.annotations (commit e893b1a) is out of scope and contract-weakening as a permanent default. mysql.annotations is included by all 8 CMPDs, so this silently disables ComponentDefinition immutability protection for the entire addon, forever — any future chart upgrade can mutate immutable CMPD fields that running clusters reference. It is an official KB annotation, but shipping it unconditionally is a hack-shaped bypass of the API's protection (review principle #2). If it exists to allow in-place CMPD iteration during this test loop, gate it ({{- if .Values.dev.skipImmutableCheck }}, default false) or keep it out of the shippable chart; either way it deserves its own PR + justification, not a ride-along in a roleProbe fix.

Blocker 2 (verify-before-merge) — kbagent contract for empty roleProbe output. The probe now returns "" + rc=0 on client timeout/error, assuming kbagent treats empty as "unknown/transient, keep last role". If kbagent instead publishes the empty result and the controller clears the pod's role label, a transient Orchestrator hiccup would de-role the primary → RW Service loses its endpoint → outage window. This must be confirmed against kbagent source or a focused test (stop Orchestrator briefly; assert role labels persist and Service endpoints don't flap) before this trades probe-timeouts for role-flapping. The PR's own boundary note says runtime replay is pending — this is the specific assertion that replay must include.

Verify-flag 1 — action budget fit. switchover.timeoutSeconds templated + client 40s + verify window 40s (parallel) + 2×3s prechecks ≈ 46s worst-case. Confirm the rendered timeoutSeconds ≥ that and stays under the kbagent action clamp; also confirm MYSQL_ORC_SWITCHOVER_VERIFY_WINDOW_SECONDS defaulting to the client budget is intended coupling.

Verify-flag 2 — ${$}-${RANDOM} tmp names in the no-timeout fallback. Two subshells forked from the same parent (the parallel candidate/current readback) inherit identical RNG state on bash < 5.1, so their first $RANDOM expansions can collide; $$ is also identical in subshells. The outer files are prefix-disambiguated, but the inner run_command_with_budget files are not. Only reachable when timeout is absent from the image — but mktemp removes the whole class.

Minor: probe path discards orchestrator-client stderr entirely (2>/dev/null) — acceptable for probe quietness, but consider logging to a file for debuggability; tee /dev/stderr in the action wrapper assumes /dev/stderr is writable in the runtime image (usually true, worth one render-run check).

Classes walked: 1 (no silent fallback hiding root cause — the converged-despite-rc!=0 path is positively verified, good), 2/3 OK, 4 flagged (empty-output sentinel meaning must be confirmed at the consumer — Blocker 2), 5/6/7 N/A-or-clean, 8 OK (rc captured immediately after substitutions). Scope: everything except commit e893b1a serves the stated ORC stability line; e893b1a should be split out.

@weicao weicao force-pushed the henry/task82-orc-roleprobe-budget branch from 2957aa3 to 86c7cea Compare July 13, 2026 13:05
@weicao weicao added the nopick Not auto cherry-pick when PR merged label Jul 13, 2026
@weicao weicao changed the base branch from helios/mysql-fixes-integration to main July 13, 2026 13:05
@weicao

weicao commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased directly onto latest main b55707f. New head: 86c7cea. Two conflicts in orc-switchover.sh were resolved by preserving main's fail-closed Orchestrator error checks and the branch's bounded client/post-check behavior. Post-rebase bash syntax, helm lint, full chart render, conflict-marker scan, and git diff --check pass. The PR base is now main; new CI is the remaining automated gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nopick Not auto cherry-pick when PR merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants